Skip to content

Conversation

@mj-kiwi
Copy link
Contributor

@mj-kiwi mj-kiwi commented Oct 8, 2025

Description

This PR updates the Transfer Window period picker to align with the latest design specifications.
Previously, the picker displayed durations in seconds or allowed manual input of seconds, which was confusing and inconsistent.

The updated picker now offers a fixed set of human-readable transfer window options for improved usability and clarity:

  • Hourly
  • Daily
  • Weekly (7 days)
  • Bi-Weekly (14 days)
  • Monthly (30 days)
  • Yearly (365 days)

These predefined options provide a more opinionated and user-friendly experience, covering the majority of use cases while maintaining consistency with the design direction.

Related issues

Fixes: #396

Manual testing steps

  1. Go to the page where the transfer period picker is displayed.

  2. Open the period picker dropdown.

  3. Verify that the options available are:

    • Hourly
    • Daily
    • Weekly (7 days)
    • Bi-Weekly (14 days)
    • Monthly (30 days)
    • Yearly (365 days)
  4. Select any option and confirm that:

    • The UI updates to show the selected duration.
    • The selection is correctly reflected in the underlying configuration or transaction preview.
  5. Verify that manual second input is no longer available.

Screenshots/Recordings

Before

Period duration shown in seconds or manual seconds input field.

After

SCR-20251009-llwi

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Adds Hourly/Biweekly/Yearly options and replaces freeform seconds with a Frequency dropdown; normalizes periodDuration to a validated number across ERC20/native periodic permissions.

  • Permissions UI (Periodic):
    • Replace periodType + freeform seconds with a single Frequency dropdown using TimePeriod options (Hourly, Daily, Weekly, Biweekly, Monthly, Yearly).
    • Remove "Other" path and numeric Duration (seconds) field; label updated to "Frequency".
  • Types/Validation:
    • periodDuration now a number (not string); Zod uses zPeriodDuration (<= 10 years) that snaps to closest TimePeriod seconds.
    • Remove periodType from contexts/metadata; update validation to accept numeric periodDuration.
  • Utils:
    • Extend TimePeriod enum and TIME_PERIOD_TO_SECONDS mapping; add getClosestTimePeriod().
  • Context/Handlers:
    • applyContext and buildContext updated to use numeric periodDuration; stop deriving/storing periodType.
  • Tests:
    • Update snapshots and validations for new dropdown/options and numeric periodDuration.
    • Add tests for getClosestTimePeriod().

Written by Cursor Bugbot for commit b9704d5. This will update automatically on new commits. Configure here.

@mj-kiwi mj-kiwi marked this pull request as ready for review October 9, 2025 03:22
@mj-kiwi mj-kiwi requested a review from a team as a code owner October 9, 2025 03:22
cursor[bot]

This comment was marked as outdated.

@mj-kiwi mj-kiwi requested a review from MoMannn October 14, 2025 22:19
… token contexts; add tests for edge cases in getClosestTimePeriod function
MoMannn
MoMannn previously approved these changes Oct 15, 2025
…ve token contexts; add zPeriodDuration definition
Copy link
Contributor

@jeffsmale90 jeffsmale90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

cursor[bot]

This comment was marked as outdated.

…emove getClosestTimePeriod function; simplify duration conversion logic
@mj-kiwi mj-kiwi requested a review from jeffsmale90 October 21, 2025 02:50
… ERC20 and native token contexts; update related tests for consistency
jeffsmale90
jeffsmale90 previously approved these changes Oct 22, 2025
Copy link
Contributor

@jeffsmale90 jeffsmale90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

One minor comment - I don't feel strongly about it though. Maybe having the additional level of normalization as it is makes this more anti-fragile.

… across ERC20 and native token contexts; update related tests for consistency
Copy link
Contributor

@jeffsmale90 jeffsmale90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 🎉

@jeffsmale90 jeffsmale90 merged commit 476e653 into main Oct 22, 2025
15 of 16 checks passed
@jeffsmale90 jeffsmale90 deleted the feat/duration-picker-improvements branch October 22, 2025 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants